Why not add this method to the EncodingProcessorState ?

done

This is something that should part of the ProcessorState. After processing a response message it should return a flag indicating that a terminal state has been reached and gRPC stream can be closed.

hmm, the idea is to decide whether to terminate the gRPC stream once the ext_proc filter receives the ProcessingResponse, i.e, at the center place inside Filter::onReceiveMessage(), as Envoy has enough information here to decide whether to terminate it then. Basically, this is determined by 1)whether the end-of-stream is received, 2) and whether Envoy needs to send more data to the ext_proc server based on filter configuration. Hooking the logic to ProcessorState will unavoidably spread the logic to each of the handling header response, handling body response(different modes), and handling trailer response, which will make it error prone and hard to maintain.

much appreciate if you could add a method comment talking about what this method is about, and comment heavily on which modes are not supported yet.

done

Thanks, could you pls also update in the change log?

do we need to consider the corner case that body is done, because we received trailers already here?

There are other corner cases:
If CONTINUE_AND_REPLACE is set, it also means we can safely half-close.

Moved the stream optimization logic to the end of the onReceiveMessage, i.e, after the ProcessingResponse is already processed. This will automatically count in CONTINUE_AN_REPLACE, as such message will modify the filter processing modes configuration, thus be counted in the check.

do we need to consider the corner case that body is done, because we received trailers already here?

This is a little bit tricky. Can we skip this corner case? If we missed closing the stream in the decoding path, the stream closing added at the encodeHeaders() will catch it and cleanup any way.

I think the end goal is to close/finish the whole side stream before destructing the filter (before mainstream tears down).
It's okay to skip, as we can fall back to the usual closure for now, but pls comment here which cases are not considered yet.

nit, there should not be a if-else here.
instead, use the state's type to get the real body response field?

I think the corner cases above apply here as well: trailers received, CONTINUE_AND_REPLACE set.

nit, there should not be a if-else here. instead, use the state's type to get the real body response field?

done

nit, you could use "const ProcessingResponse&" here to save some dereferences.

done

if the ext_proc sends invalid response, e.g. requestHeaders on Body events, it's a bad state that would cause stream close already.
Shall we evaluate all the cases after the response is processed?

Moving the stream closing logic to the end of onReceiveMessage() automatically count in this as well as stream_ become nullptr in these conditions.

other case: body mode confgiured, but trailers received.

trailer mode set, but EoS seen in headers or Body already

I added the cases you mentioned in the TBD list in the PR description, please take a look. The goal of this PR is to get the normal case optimization working, then we can deal with the corner cases in the future enhancement. BTW, if some corner cases are missed in the decoding path, the stream closing logic added in encodeHeaders() will catch them all.

let's add the TODO comments in code. those are not "corner cases" IMHO.
Since you started this, I hope you could finish it :)

what if trailers are configured?

Here we detect EoS is received with body, so no trailers in this request. Thus even filter is configured to send trailers, the external processing in this direction is completed.

nit: just return

Thanks, could you pls also update in the change log?

I think the end goal is to close/finish the whole side stream before destructing the filter (before mainstream tears down).
It's okay to skip, as we can fall back to the usual closure for now, but pls comment here which cases are not considered yet.

not sure if it's my bad taste, but we can ignore all these has_XXX IMHO.

If we do not check these has_xxx, and if they are not set, the code will rely on the default behavior of these messages, which is error-prone.

I don't fully follow why is_last_body_resp is an input parameter, could it be inferred using the response?

Good question!

For FULL_DUPLEX_STREAMED mode, it can be inferred from the response.

For STREAMED mode, the response does not contain EOS. In this case, the EOS info is stored in the Envoy chunk_queue element. However, as we are doing the stream_closing after the response message already processed, i.e, the chunk_queue element is already popped by then. So, for STREAMED mode, we have to detect whether this is_last_body_resp before we process the response message. Then passing the is_last_body_resp to the stream_closing logic.

this didn't consider request trailers: if EoS is set by decodeTrailers, last_response can be inferred by trailers mode and the queue state for streamed mode.

We can do this in future PR. Some of the cases are listed here: #41425 (comment).

IMHO, in this PR, we just close the side stream for the cases we are 100% sure. For corner cases, they will be taken care of by the fall back mechanism which is filter onDestroy(). We don't have to be perfect in this PR. We can iterate and enhance. From another angle, it's probably bad to accidently close a stream we should not close. So, let's do this step-by-step.

if you do the evaluation after the response is "processed". then no need to check on state.chunkQueue().queue().front()->end_stream().
queue empty + state.completeBodyAvailable() works the same

queue empty + state.completeBodyAvailable() is ambiguous here. It may mean two different cases:

1. The body chunk is the last body with EoS = true;
2. or The body chunk is the last body with EoS = false, and trailer is received.

For case 1), it is equivalent to state.chunkQueue().queue().front()->end_stream(), which we can for sure close the stream. However, for case 2), it needs cross check the trailer processing mode to determine whether we can close the stream. We can do this in a follow up PR.

if body_replaced_ is true, there is no more message to send as well.

The logic considered body_replaced_ already as CONTINUE_AND_REPLACE processing will also end up with

body_mode_ = ProcessingMode::NONE;
send_trailers_ = false;

So, the logic can capture this case.

if not in this CL, comment on the case that's discussed on the request path

can we have a parameterized test that use enumerate through the response types and possible "shortcut switches" (i.e. eos-in-mxn-mode, CONTINUE_AND_REPLACE in header response etc) and check whether an early half-close is sent?
I know this is a lot of cases, but with this set up, we will have confidence in the changes we are making in this PR and upcoming PRs.

Please check the changes in filter_test.cc and filter_full_duplex_test.cc. The config_->stats().streams_closed_ counters are specially checked to make sure we are now closing the streams ASAP in those cases.